Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add 2 rust analyzer LSP extensions #2776

Merged
merged 2 commits into from
May 8, 2021

Conversation

petr-tik
Copy link
Contributor

@petr-tik petr-tik commented Apr 12, 2021

Implement OpenCargoToml - takes a universal arg to open in another window similar to lsp-clangd-find-other-file.
Still has some outstanding comments for code review.

Add the Related Tests interactive function - for some reason this rust-analyzer endpoint doesn't return Runnable[] but instead a list of key value mappings, where key is always "runnable" and value is the Runnable, hence I couldn't find many opportunities for code reuse with other lsp-rust-analyzer-*-runnable methods

Extract the common code to run a given runnable in cargo-process-mode and use it in lsp-rust-analyzer-run as well as lsp-rust-analyzer-related-tests. Tested manually in a local checkout of rust-analyzer (of course), didn't add any tests, as there isn't a sample rust project.

Misc code tidy-up to pacify flycheck

@github-actions github-actions bot added client One or more of lsp-mode language clients documentation rust labels Apr 12, 2021
@ericdallo ericdallo requested a review from brotzeit April 12, 2021 22:36
clients/lsp-rust.el Outdated Show resolved Hide resolved
@petr-tik
Copy link
Contributor Author

@brotzeit should I add any documentation on these 2 functions or is documentation auto-generated from the code?

clients/lsp-rust.el Outdated Show resolved Hide resolved
clients/lsp-rust.el Outdated Show resolved Hide resolved
@brotzeit
Copy link
Member

brotzeit commented Apr 13, 2021

@brotzeit should I add any documentation on these 2 functions or is documentation auto-generated from the code?

@petr-tik seems to me that only documentation of defcustoms is generated for the wiki.

@ericdallo
Copy link
Member

Yes, only documentation of defcustom are auto generated, otherwise you should edit docs/manual-language-docs/lsp-rust.md @petr-tik

@petr-tik
Copy link
Contributor Author

Yes, only documentation of defcustom are auto generated, otherwise you should edit docs/manual-language-docs/lsp-rust.md @petr-tik

Thanks for clarifying. Since there were no comments about function names or UX, I think I can record the gifs, while changing the implementation

@petr-tik petr-tik force-pushed the rust-analyzer-lsp-extensions branch 2 times, most recently from c8613db to 919c531 Compare May 3, 2021 21:02
@petr-tik petr-tik requested a review from kiennq May 3, 2021 21:03
clients/lsp-rust.el Outdated Show resolved Hide resolved
@petr-tik
Copy link
Contributor Author

petr-tik commented May 6, 2021

Not sure if this belongs here, maybe doom?

I realised that one of those lsp extensions can replace a function defined in rustic-mode.

(if (and (fboundp #'rustic-open-dependency-file)
         (fboundp #'lsp-rust-analyzer-open-cargo-toml))
    (advice-add 'rustic-open-dependency-file :override #'lsp-rust-analyzer-open-cargo-toml))

thoughts?

@yyoncho
Copy link
Member

yyoncho commented May 7, 2021

@petr-tik rustic has lsp module, right?

@petr-tik
Copy link
Contributor Author

petr-tik commented May 7, 2021

@petr-tik rustic has lsp module, right?

Indeed - thanks for pointing out this basic omission of mine. I can contribute my humble patch there, should you decide that this PR is worthy of merging and they deem my little piece of advise a deserving addition.

In the meantime, i shall await your review and verdict on this attempt at extending the already glorious lsp-mode.

Copy link
Member

@yyoncho yyoncho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good: few minor nits.

clients/lsp-rust.el Outdated Show resolved Hide resolved
clients/lsp-rust.el Outdated Show resolved Hide resolved
clients/lsp-rust.el Outdated Show resolved Hide resolved
@yyoncho
Copy link
Member

yyoncho commented May 7, 2021

I can contribute my humble patch there,

It should be in lsp-mode since there is rust-mode as well.

@petr-tik
Copy link
Contributor Author

petr-tik commented May 7, 2021

I can contribute my humble patch there,

It should be in lsp-mode since there is rust-mode as well.

I can leave it at the bottom of the file then? I am just not sure if people want a behind-the-scenes change in interactive functions

Takes a universal arg to open in another window similar
to lsp-clangd-find-other-file

Implement the related tests ra extension

Extract the common code to run a given runnable in
cargo-process-mode and use it in ~lsp-rust-analyzer-run~
as well as =lsp-rust-analyzer-related-tests=
@yyoncho
Copy link
Member

yyoncho commented May 7, 2021

I can leave it at the bottom of the file then? I am just not sure if people want a behind-the-scenes change in interactive functions

It should not be part of lsp-mode. It should be part of rustic and it should run either OOTB or it should be behind a config option.

@yyoncho
Copy link
Member

yyoncho commented May 7, 2021

Can you fix the conflicts?

@petr-tik petr-tik force-pushed the rust-analyzer-lsp-extensions branch from 919c531 to d3d00fc Compare May 7, 2021 20:36
@petr-tik
Copy link
Contributor Author

petr-tik commented May 7, 2021

Can you fix the conflicts?

Have addressed your careful review to the best of my limited ability and resolved the conflict in the changelog.

Only the ivy-specific comment left - I think the rest of the PR is ready.

extend the rust-analyzer protocol with relevant entries

add a review comment about the preview function

Add manual documentation and gifs for both interactive functions
@petr-tik petr-tik force-pushed the rust-analyzer-lsp-extensions branch from d3d00fc to f2ac5e4 Compare May 7, 2021 20:49
@yyoncho yyoncho merged commit fd05f0d into emacs-lsp:master May 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client One or more of lsp-mode language clients documentation rust
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants